In the first version of LOR, we have the following code which avoid this issue you pointed out.

  private val weightsArray = weights match {
     case dv: DenseVector => dv.values
     case _ =>
       throw new IllegalArgumentException(
         s"weights only supports dense vector but got type ${weights.getClass}.")
   }

I think order approach will be more efficient since toArray is only called once (you can add the case for sparse), and for sparse initial coefficients, we will not convert from sparse to dense again and again.

This can be a future work. With L1 applied, the coefficients can be very sparse, so we can compress the coefficients for each iteration, and have specialized implementation for UpdateInPlace.

The above check got us into trouble because if we don't add @transient lazy val then we'll serialize the coefficients. The call to toArray is really just a small bit of pointer indirection, and while I agree it is not great to call it every time, the extra function call should pale in comparison to the O(numClasses * numFeatures) ops we do in the method.

That said, I'm ok with either solution, just wanted to point out the pros/cons of each. Let me know what you think, thanks for reviewing!

Another thought is that by tagging the class member as @transient lazy val we are at least making this hidden problem more explicit. I think using the transient method makes it a bit less likely that someone will come along and make a change in the future that serializes the coefficients. I'll plan to update it here with the transient tag then.

My concern is that if coefficients is sparse, we are not just doing the pointer indirection but creating a new dense array from sparse matrix. I know we always pass in a dense matrix so this will not be an issue now, but being said that, in the following code, if we call compress in the coefficients, we may be able to broadcast a smaller object when L1 is applied or in the initial iteration that most of the elements in coefficients are zero.

https://github.com/sethah/spark/blob/3bea389f6780e1fd0385fbe26954fa4f59b69e37/mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala#L1674

My concern is that if coefficients is sparse, we are not just doing the pointer indirection but creating a new dense array from sparse matrix. I know we always pass in a dense matrix so this will not be an issue now, but being said that, in the following code, if we call compress in the coefficients, we may be able to broadcast a smaller object when L1 is applied or in the initial iteration that most of the elements in coefficients are zero.

https://github.com/sethah/spark/blob/3bea389f6780e1fd0385fbe26954fa4f59b69e37/mllib/src/main/scala/org/apache/spark/ml/classification/LogisticRegression.scala#L1674

Can you have the type of coefficientsArray so people can clear know that it's a primitive array?

Yeah, I'll update it.

I think we should handle sparse coefficients for further performance improvement. But not in this PR.